Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring CDEPS inline capability to CMEPS #2028

Conversation

uturuncoglu
Copy link
Collaborator

@uturuncoglu uturuncoglu commented Dec 6, 2023

PR Author Checklist:

  • I have linked PR's from all sub-components involved in section below.
  • I am confirming reviews are completed in ALL sub-component PR's.
  • I have run the full RT suite on either Hera/Cheyenne AND have attached the log to this PR below this line:
  • I have added the list of all failed regression tests to "Anticipated changes" section.
  • I have filled out all sections of the template.

Description

This PR aims to bring CDEPS inline capability to CMEPS to support regional configurations like HAFS. The more information related to the implementation can be found in the following presentation: https://docs.google.com/presentation/d/1Tk76zlsRiT7_KMJiZsEJHNvlHciZJJBBhsJurRMxVy4/edit#slide=id.g2631444dbf7_0_5

Commit Message

Developed and enabled using CMEPS with inline CDEPS capability for UFS regional coupling.
Two HAFS regional moving nest atm_ocn_wav coupling regression tests were added to test this capability.
Note: This is a collaborative effort among the ESMF team, EMC/EIB, and EMC hurricane project team.

Linked Issues and Pull Requests

Associated UFSWM Issue to close

Subcomponent Pull Requests

Blocking Dependencies

Subcomponents involved:

Anticipated Changes

Input data

  • No changes are expected to input data.
  • Changes are expected to input data:
    • New input data.
    • Updated input data.

Two new subdirs (MOM6_regional_input_data and CDEPS_input_data) were added inside the FV3_hafs_input_data dir. Adding these two subdirs will not affect any existing RTs. They are only used for the newly added two RTs in this PR.

With that, please help to copy over the following dirs:
/scratch1/NCEPDEV/stmp4/Bin.Liu/hafs/NEMSfv3gfs/input-data-20221101/FV3_hafs_input_data/MOM6_regional_input_data
/scratch1/NCEPDEV/stmp4/Bin.Liu/hafs/NEMSfv3gfs/input-data-20221101/FV3_hafs_input_data/CDEPS_input_data
inside the existing NEMSfv3gfs/input-data-20221101/FV3_hafs_input_data/, and then populate them to all supported platforms. Again, no need to change the input-data-20221101 version. Thanks!

Regression Tests:

  • No changes are expected to any existing regression test.
  • Changes are expected to the following two new added tests:
Tests effected by changes in this PR: The following two new RTs were added for HAFS regional moving nest coupling through CMEPS with inline CDEPS: hafs_regional_storm_following_1nest_atm_ocn_wav_inline and hafs_regional_storm_following_1nest_atm_ocn_wav_mom6

Libraries

  • Not Needed
  • Needed
    • Create separate issue in JCSDA/spack-stack asking for update to library. Include library name, library version.
    • Add issue link from JCSDA/spack-stack following this item
Code Managers Log
  • This PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR.
  • Move new/updated input data on RDHPCS Hera and propagate input data changes to all supported systems.
    • N/A

Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • Jet
    • Gaea
    • Cheyenne
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
    • Completed
  • opnReqTest
    • N/A
    • Log attached to comment

This is initially tested under UFS Weather Model and did not change the answer for existing HAFS RTs and cpld_control_p8 cases. More test will be performed with both UFS Weather model. The PR is also tested on NCAR's CESM model and does not change the answers and able to run the existing configurations.

@DeniseWorthen
Copy link
Collaborator

@uturuncoglu We're adding a Commit Message requirement to the PRs. Please add one in the space provided.

@uturuncoglu
Copy link
Collaborator Author

@BinLiu-NOAA @DeniseWorthen @junwang-noaa @jedwards4b I made couple of changes in the CMEPS side to allow filling exchange fields with all data in the first coupling time step and it seems working fine. I introduce set of new mediator namelist option to make it configurable. So, for example I am setting following in the nems.configure for ocean and wave,

  ocn_use_data_first_import = .true.
  wav_use_data_first_import = .true.

By default the values are .false. and it is only usable when CDEPS Inline feature activated. I also modify the wave and ocean prep phases to get the data and pass it to the components. I pushed all to CMEPS fork. What is next;

  • I am plaining to work on component side (WW3 and HYCOM) to fix their import states since as I see there was some issue in the merge code in there. My aim is to be sure that the fields passed by the mediator is actually used by the components.
  • I still need to to extensive testing with the existing configurations of UFS and CESM.
  • I need to sync my branches with head of develop in UFS Weather Model side. As I know there are lots of changes in terms of renaming templates etc. So, it would be nice to sync the model before diverging too much.
  • I am also planning to point CDEPS PR without additional changes related with the UFS specific data modes since those are not pushed to ESCOMP before and without CIME interface it will be hard to push them - so, new CDEPS fork will be based on ESCOMP version with additional changes that I Meade for source and destination mask

Anyway, I think we are very close to finalize the development and make PR's ready for review. I'll update you when it is ready.

@uturuncoglu
Copy link
Collaborator Author

@BinLiu-NOAA @DeniseWorthen @junwang-noaa I think logic in the wav_import_export.F90 file that creates the merge mask does not allow to get mediator provided field for initial time step if merge_import is set to true.

(https://github.com/NOAA-EMC/WW3/blob/02693d837f2cd99d20ed08515878c2b5e9525e64/model/src/wav_import_export.F90#L1426)#L1426

In this case, it will always read its own data file rather then provided one from mediator since firstCall is always true and it sets the global_mask to all zero. So, it prevents to use mediator provided data even if wav_use_data_first_import is set to true.

https://github.com/NOAA-EMC/WW3/blob/02693d837f2cd99d20ed08515878c2b5e9525e64/model/src/wav_import_export.F90#L1369

As I see from the code, the logic is fine for the second time step since the mask will be set to all 1.0 (there won't be any value in import field greater or equal than fill value since it is already filled in mediator side) and it will use the mediator provided completed field.

So, it would be fine to set merge_import as false along with wav_use_data_first_import = .true since the mediator will provide all data without any unmapped region as well as the data for atmospheric forcing at initial coupling time step (after I bring that capability). I tested this configuration and set merge_import as false for WW3 and worked as expected. So, I think there is no need to use merge_import in the WW3 side along with the new feature.

@uturuncoglu
Copy link
Collaborator Author

@BinLiu-NOAA Maybe I am missing something in here but it seems that merge_import configure option that is defined in the ocean section of nems.configure is not used by HYCOM. There is no any reference in the HYCOM code to get (and use) merge_import attribute. So, it seems merging is always active in HYCOM side and the code just check the import field with fill_value to activate it. Please correct me if I am wrong but instead of using merge_import HYCOM uses cpl_merge internal variable and it seems it is always active.

I also checked the HYCOM code and it seems that cpl_merge variable only used for latent and sensible heat fluxes and replaces large values (fill_value) with the ones that are calculated internally by using some bulk formulas based on wind, temperature and air density variables (it is in HYCOM/thermf.F90) and it is setting zero for other variables like stress components etc. Anyway, again let me know if I am wrong.

Based on my finding, it would be also fine to set merge_import as false (maybe there is no need to set it since it is not used by the model and needs to be removed from the nems.configure template). HYCOM is fine and running without any issue even if I set merge_import to false and disable CDEPS inline in CMEPS run sequence. So, this also confirms that this option is not used by HYCOM and can be removed from the template files.

It seems that if mediator handles the unmapped region, HYCOM will be fine with it and runs without any issue since merge is based on checking fill_value and if there is no data like fill_value it won't do anything. It would be also nice to set skip_first_import to false since mediator also able to provide data in the first coupling tilmestep.

Anyway, I tested the configuration by setting merge_import and skip_first_import to false in HYCOM side and activating CMEPS inline capability. It seems simulation runs without any issue (I did not check the model output for input forcing, if you could tell me how could I process the HYCOM output to check wind stress and other flux components that are provided by the atmosphere that would be great - or you could test the configuration and confirm that they are all fine). I modified template namelist file for the hafs_regional_atm_ocn_wav_inline test and set all merge_import etc. to zero except FV3 since we are not providing roughness length via CDEPS Inline at this point. Please have a fresh clone of the model and test the configuration again. In the mean time, I'll check the existing configuration to be sure the development in CMEPS and CDEPS side does not affect anything.

@uturuncoglu
Copy link
Collaborator Author

@DeniseWorthen @BinLiu-NOAA @junwang-noaa I run full RT suite on Orion and I have some tests failed but they seems failed due to some issue in mediator aoflux calculation. At this point, my CMEPS fork is based on ESCOMP and it seems the NOAA-EMC fork is 1 commit ahead and 512 commits behind of ESCOMP/CMEPS:main. So, this might trigger issue. I am not sure what is the best way to bring this in. We could merge this with ESCOMP and then we could update CMEPS in NOAA-EMC fork and push changes related with the mediator CCPP host model (clm lake related) to ESCOMP. But I am not sure this solves the failed tests. Maybe we could try to run tests by syncing NOAA CMEPS fork with ESCOMP without any modification related to CDEPS Inline to see what happens. Anyway, let me know what you think?

Additionally, my CDEPS fork is based on NOAA-EMC and as I know you did not push extra data modes to ESCOMP yet. Maybe I could create another fork that has only work related with source and destination masks to merge with ESCOMP CDEPS and then you could sync NOAA-EMC fork with the ESCOMP.

@DeniseWorthen
Copy link
Collaborator

@uturuncoglu The commit history I cannot explain but the code itself can be compared and there are only a few differences related to some recent changes for gusts at ESCOMP.

Where are your orion runs? I can take a look.

@uturuncoglu
Copy link
Collaborator Author

@DeniseWorthen My HAFS related run directory is in /work/noaa/nems/tufuk/INLINE/hafs_regional_atm_ocn_wav_intel. The RT are in /work/noaa/stmp/tufuk/stmp/tufuk/FV3_RT/rt_78337/. For example, you could check following log file, /work/noaa/stmp/tufuk/stmp/tufuk/FV3_RT/rt_78337/datm_cdeps_control_cfsr_intel/PET00.ESMF_LogFile.

@DeniseWorthen
Copy link
Collaborator

@uturuncoglu I want to be sure which CMEPS branch you're using. The UWM PR shows feature/cdeps_inline but that branch seems to have been last updated 2 months ago whereas branch feature/inline is only 4days old.

@uturuncoglu
Copy link
Collaborator Author

@DeniseWorthen I am using feature/inline branch in my fork (https://github.com/uturuncoglu/CMEPS). As I remember it is based on ESCOMP.

@DeniseWorthen
Copy link
Collaborator

DeniseWorthen commented Jan 3, 2024

@uturuncoglu I merged the current escomp/main into emc/develop and I got the same error shown in your datm_cdeps_control test

20240103 074019.408 INFO             PET00 (med_aofluxes_init): called
20240103 074019.409 ERROR            PET00 ESMCI_Container_F.C:168 ESMCI::Container::get() Invalid argument  - key does not exist
20240103 074019.409 ERROR            PET00 ESMCI_Container_F.C:448 c_esmc_containergetfield() Invalid argument  - Internal subroutine call returned Error
20240103 074019.409 ERROR            PET00 ESMF_Container.F90:636 ESMF_ContainerGetField() Invalid argument  - Internal subroutine call returned Error
20240103 074019.409 ERROR            PET00 ESMF_FieldBundle.F90:11525 ESMF_FieldBundleGetItem() Invalid argument  - Internal subroutine call returned Error

I suspect this is related to the new gust parameterization added for CESM.

@DeniseWorthen
Copy link
Collaborator

@uturuncoglu I made these changes after merging ESCOMP/main and the datm test now runs. You can see my code at
/work/noaa/marine/dworthen/ufs-weather-model/CMEPS-interface/CMEPS/mediator

diff --git a/mediator/esmFldsExchange_ufs_mod.F90 b/mediator/esmFldsExchange_ufs_mod.F90
index d736717..a93a8ff 100644
--- a/mediator/esmFldsExchange_ufs_mod.F90
+++ b/mediator/esmFldsExchange_ufs_mod.F90
@@ -151,14 +151,6 @@ contains
        call addfld_ocnalb('So_anidf')
     end if

-    ! Advertise the ocean albedos. These are not sent to the ATM in UFS.
-    if (phase == 'advertise') then
-       call addfld_ocnalb('So_avsdr')
-       call addfld_ocnalb('So_avsdf')
-       call addfld_ocnalb('So_anidr')
-       call addfld_ocnalb('So_anidf')
-    end if
-
     !=====================================================================
     ! FIELDS TO ATMOSPHERE (compatm)
     !=====================================================================
diff --git a/mediator/med_phases_aofluxes_mod.F90 b/mediator/med_phases_aofluxes_mod.F90
index cc62bbd..b074cda 100644
--- a/mediator/med_phases_aofluxes_mod.F90
+++ b/mediator/med_phases_aofluxes_mod.F90
@@ -1597,8 +1597,6 @@ end subroutine med_aofluxes_map_ogrid2xgrid_input
        if (chkerr(rc,__LINE__,u_FILE_u)) return
        call fldbun_getfldptr(fldbun_a, 'Sa_shum', aoflux_in%shum, xgrid=xgrid, rc=rc)
        if (chkerr(rc,__LINE__,u_FILE_u)) return
-       call fldbun_getfldptr(fldbun_a, 'Faxa_rainc', aoflux_in%rainc, xgrid=xgrid, rc=rc)
-       if (chkerr(rc,__LINE__,u_FILE_u)) return
     end if

     ! extra fields for ufs.frac.aoflux
@@ -1710,8 +1708,6 @@ end subroutine med_aofluxes_map_ogrid2xgrid_input
     if (chkerr(rc,__LINE__,u_FILE_u)) return
     call fldbun_getfldptr(fldbun, 'So_duu10n', aoflux_out%duu10n, xgrid=xgrid, rc=rc)
     if (chkerr(rc,__LINE__,u_FILE_u)) return
-    call fldbun_getfldptr(fldbun, 'So_ugustOut', aoflux_out%ugust_out, xgrid=xgrid, rc=rc)
-    if (chkerr(rc,__LINE__,u_FILE_u)) return
     call fldbun_getfldptr(fldbun, 'Faox_taux', aoflux_out%taux, xgrid=xgrid, rc=rc)
     if (chkerr(rc,__LINE__,u_FILE_u)) return
     call fldbun_getfldptr(fldbun, 'Faox_tauy', aoflux_out%tauy, xgrid=xgrid, rc=rc)
@@ -1736,6 +1732,12 @@ end subroutine med_aofluxes_map_ogrid2xgrid_input
        allocate(aoflux_out%evap_18O(lsize)); aoflux_out%evap_18O(:) = 0._R8
        allocate(aoflux_out%evap_HDO(lsize)); aoflux_out%evap_HDO(:) = 0._R8
     end if
+    if (add_gusts) then
+       call fldbun_getfldptr(fldbun, 'So_ugustOut', aoflux_out%ugust_out, xgrid=xgrid, rc=rc)
+       if (chkerr(rc,__LINE__,u_FILE_u)) return
+    else
+       allocate(aoflux_out%ugust_out(lsize)); aoflux_out%ugust_out(:) = 0._R8
+    end if

   end subroutine set_aoflux_out_pointers

@uturuncoglu
Copy link
Collaborator Author

@DeniseWorthen That is great. Thanks for your help. Let me test in my side too. Do you want me to add this fix to the PR. I am not sure how urgent to have this fix in UFS side.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Feb 5, 2024

@uturuncoglu @binli2337 can you update cmeps feature branch hash: uturuncoglu/CMEPS@c9c4c23?

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Feb 5, 2024

@uturuncoglu @binli2337 can you update cmeps feature branch hash: uturuncoglu/CMEPS@c9c4c23?

@BinLiu-NOAA you are quick! thanks!

@BinLiu-NOAA
Copy link
Contributor

@jkbk2004 @BrianCurtis-NOAA @uturuncoglu @DeniseWorthen @binli2337
As being discussed under NOAA-EMC/CMEPS/pull/110, we agreed to bring in the CMEPS fix now.

Unfortunately, in this case, we will need to recreate/update the two HAFS new RTs baseline, and then rerun the full RTs against the baseline.

Much appreciated for all your help and patience to deal with this critical PR development needed for HAFSv2 code freeze.

@BrianCurtis-NOAA
Copy link
Collaborator

Acorn was almost done before they cut the connection on me for maintenance. With the fact that WCOSS2 finished and Acorn had 0 failed tests minus the one that was finishing, Acorn can be "skipped" for this 2nd round of testing.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Feb 5, 2024

Derecho is not available today. @zach1221 @FernandoAndrade-NOAA Can you pick up from here? I think we are good to go.

@DeniseWorthen
Copy link
Collaborator

CMEPS is merged (624920d); CDEPS was merged previously (3d7067a)

@BinLiu-NOAA
Copy link
Contributor

Thanks! Both CMEPS and CDEPS submodules should be up to date now. Much appreciated for the speedy RT processing after the bug fixes! Thanks!

@FernandoAndrade-NOAA FernandoAndrade-NOAA merged commit 8c9339f into ufs-community:develop Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Baselines New baselines will be added to project. New Input Data Req'd This PR requires new data to be sync across platforms Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop regional ufs-weather-model coupling capability with CMEPS and inline CDEPS
10 participants